-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add tooltips to editor tabs #456
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Screen.Recording.2024-08-07.at.12.43.59.movsmall change to close tooltip on tab click incoming shortly |
I have read the CLA Document and I hereby sign the CLA |
I don't love that I am manually setting the "openTabKey" to the first item in the array on load but have not found a smarter way to determine whats currently open. Open to ideas! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, already.
Just a few small adjustments. :)
title: string | ||
children: React.ReactNode | ||
} | ||
export const IconWrapper = ({ tabKey, openTabKey, title, children }: IconWrapperProps): React.JSX.Element => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add the tooltip, when the Tab has focus via keyboard? :)
Just want make sure that we meet the a11y requirements, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2024-08-08.at.16.46.22.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I am not able to access each tab element separately (because of Tabs.TabPane soon being deprecated)
I have made do with the onFocus/ onBlur combined with some event.target.id text matching.
When someone is using the Keyboard to tab through the tabs and then switches to navigate via mouse, two tooltips can show at once which I think makes sense (see vid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'd agree that it makes sense to show 2 tooltips in that case...
|
||
const onTabClick = (key: string): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also work with the keyboard? Otherwise we maybe could use the onChange listener instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tested it and it also work for keyboard navigation. I do think the onChange handler seems more fitting though. Changed, Thanks!
@@ -30,11 +31,27 @@ export interface IEditorTabsProps { | |||
showLabelIfActive?: boolean | |||
} | |||
|
|||
export interface IconWrapperProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this interface can be removed? Seems like it was moved to the tooltip component already? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! :)
Just one more organizational topic for the PRs.
The PR title should always be written like a commit message, since it will be one after we squash it. ;D
@xIrusux, very nice first PR 🥇 nice to have you in the team 🥳 🎉 |
Changes in this pull request
Add tooltips to editor tabs
Resolves #435
Additional info
Resorted to writing a custom icon wrapper to add and remove the "open" property on hover.
Unfortunately Antd does not have an out of the box tooltip solution for the way we use their Tabs component (i.e. we remove the label text on click).
I explored the option to add the tooltip on Tabs.TabPane but later read that it's (soon to be?) deprecated.